Codecov Report

Merging #1837 (730071f) into main (5180b4f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1837   +/-   ##
=======================================
  Coverage   91.54%   91.54%           
=======================================
  Files         203      203           
  Lines       10945    10945           
=======================================
  Hits        10020    10020           
  Misses        925      925           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bouweandela this is possible solution to #505.

In your comment you mentioned also adapting frequencies accordingly. I thought about this some time now and found several possible solutions for this:

1. Use a similar function than the one used in the CMOR checks. This one checks all time points and might be very expensive, especially if we do not make any assumptions about the cube's frequency and let the code check everything.
2. Only check the first two points. This is much cheaper, but might lead to wrong results.
3. Somehow couple this to the preprocessors used. E.g., if someone uses annual_statistics, we know that the data is yearly afterwards. This might not be straightforward to implement and might get confusing if multiple of the units-changing preprocessors are used.

In addition, I have some questions:

• How do we deal with data that does not have a time dimensions anymore (e.g., after running climate_statistics with period=full). Use fx? Use None?
• How do we deal with climatologies (e.g., after running climate_statistics with period=monthly? For monthly climatologies, do we simply use mon? I am not entirly sure as this is different to a standard monthly time series.

All these problems lead me to the conclusion that it might be best to tackle the frequency update at a later stage. This PR solves an actual existing problem, whereas no one complained about wrong frequencies before. So maybe just open an issue about this and merge this without adapting the frequencies?

@ESMValGroup/esmvaltool-coreteam any opinions?

My idea wasn't nearly as advanced as that: i thought we could just update the frequency in metadata.yml using the value of the frequency or time_frequency attribute on the cube.

Prepeocessors who change the frequency or any of the other attributes should then take care of updating the appropriate cube attributes.

Right - that's much easier indeed. Two questions on this:

1. Should I update all time preprocessors with the frequency attribute in this PR? If yes, I would need an answer to my questions above.
2. Are you proposing to update the product attributes from all cube attributes? I guess this can be a little bit dangerous, for example, if a dataset uses a key that we also use in the raw data (e.g., project) then we might accidentally incorrectly overwrite the product attributes.

Should I update all time preprocessors with the frequency attribute in this PR? If yes, I would need an answer to my questions above.

I would leave that for a new pull request.

Are you proposing to update the product attributes from all cube attributes? I guess this can be a little bit dangerous, for example, if a dataset uses a key that we also use in the raw data (e.g., project) then we might accidentally incorrectly overwrite the product attributes.

No, just the ones that are wrong after preprocessing.

All right, implemented.

One remaining question: Currently, if a cube property (e.g., cube.standard_name) is None, the metadata is update with an empty string. This is in line with our current procedure in the CMOR checks.

However, in the light of preprocessors that completely remove cube metadata (#651), this might lead to unexpected results. Would it be safer to just not update the corresponding values if it's None?

Corresponding frequency issue: #1840

thanks a lot @schlunma and @bouweandela 🍺